/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Mostly solid work — correctness and test coverage are good, one security gap worth fixing before merging.

Findings

🔒 Security Concerns

src/safeoutputs/add_pr_comment.rs:91 — repository not validated with is_safe_path_segment

AddPrCommentParams::validate() calls reject_pipeline_injection on repository, but that check only blocks ADO expressions/pipeline commands. It does not reject path traversal components like ../. repository_checkout_dir then passes this value directly to source_directory.join(repository), which can escape the build workspace:

// Current (line 91) — only blocks ADO injection, not traversal
reject_pipeline_injection(&self.repository, "repository")?;

// Fix — consistent with mcp.rs:228 which already uses this
if !self.repository.is_empty() {
    ensure!(
        crate::validate::is_safe_path_segment(&self.repository),
        "repository must be a safe path segment (no '..', '/', or '\\'), got: '{}'",
        self.repository
    );
}

There is a runtime defense at execute_impl line ~345 (lookup_allowed_repository fails on unknown aliases), so this isn't an immediate exploit in normal use. But: (a) the Validate check runs before execution and should be the first line of defense, (b) mcp.rs already uses is_safe_path_segment for the same field type for consistency, and (c) an operator-defined checkout alias containing .. (unusual but possible) would bypass the runtime gate while the path traversal guard in build_inline_thread_context anchors to repo_root (the already-escaped path) rather than ctx.source_directory.

⚠️ Suggestions

src/safeoutputs/add_pr_comment.rs:231–235 — path guard anchored to repo_root, not source_directory

The starts_with(&canonical_root) check (line 234) guards against traversal within repo_root, but canonical_root is derived from the (potentially traversed) repo_root itself, not from ctx.source_directory. As defense-in-depth, also checking against the workspace root would be stronger:

let canonical_workspace = ctx.source_directory.canonicalize()
    .context("Failed to canonicalize source directory")?;
ensure!(
    canonical.starts_with(&canonical_workspace),
    "Inline comment file resolves outside the build workspace"
);

✅ What Looks Good

• UTF-16 counting: Using str::lines() (which strips \r\n) before encode_utf16().count() correctly handles CRLF files — nice catch.
• Fenced block parser: find_matching_fence_end returning next_line_start (which includes the closing fence's \n) means the protected range is complete. The empty-iterator all() case for a bare ``` closer is handled correctly.
• Unmatched backtick safety: The test test_escape_html_tags_unmatched_inline_backtick_does_not_disable_escaping is exactly the right edge case to have.
• Emoji test: Using ab😀 (1 + 1 + 2 UTF-16 code units = 4, +1 = offset 5) is a well-chosen test — hits the surrogate pair path.
• Error handling: File read errors and line-out-of-range all go through ExecutionResult::failure — no panics on bad agent input.
• ensure! ordering in build_inline_thread_context: Input validation (start_line > 0, end_line > 0, start_line ≤ end_line) is checked before any I/O. ✓

Generated by Rust PR Reviewer for issue #1104 · 322.1 AIC · ⌖ 12.1 AIC · ⊞ 34.2K · ◷
_{Comment /rust-review to run again}

@copilot adfress review comments

@copilot adfress review comments

Addressed in a97ed85. The fix now rejects traversal-style repository selectors, resolves inline comment checkout roots through the allowed repository map, and enforces build-workspace containment for anchored files.

/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Mostly solid — the UTF-16 offset fix and markdown-aware sanitization are well-executed. Two things worth addressing before merge: a logic-duplication risk and sparse test coverage on the new lookup path.

Findings

⚠️ Suggestions

repository_checkout_dir duplicates and diverges from lookup_allowed_repository
(src/safeoutputs/add_pr_comment.rs:211)

The new function re-implements the exact same three-way lookup logic (exact alias key → case-insensitive full value → case-insensitive trailing name) that already lives in mod.rs::lookup_allowed_repository. The duplication is necessary because repository_checkout_dir needs the alias key (to build the path), while lookup_allowed_repository returns the value (the ADO repo name). But if a 4th match type is ever added to the canonical function, repository_checkout_dir silently won't pick it up.

Consider extracting a private lookup_allowed_repository_key(input, map) -> Option<&str> helper that returns the key (alias), then both the canonical resolver and the new checkout-dir function can delegate to it.

Sparse test coverage for repository_checkout_dir
(src/safeoutputs/add_pr_comment.rs:886)

test_repository_checkout_dir_resolves_full_repository_name_to_alias_path only exercises lookup path 3 (case-insensitive full value). Lookup path 1 (exact alias key) and the "self" / empty-string fast path are untested. Since this function is the gatekeeper for which on-disk directory gets read, coverage gaps here are higher risk than usual.

collect_inline_code_ranges restricts inline code to single lines — CommonMark deviation
(src/sanitize.rs:355)

let inline_code_boundary = input[i..end]
    .find('\n')
    .map(|offset| i + offset)
    .unwrap_or(end);

CommonMark permits inline code spans to wrap a single newline (`foo\nbar` is valid). With this implementation, both backticks would be treated as unmatched, and any HTML inside the span would be escaped. This is the safe direction (no injection risk), but it could confuse authors who embed < or > in multi-line inline code in their PR comments. Worth a comment explaining the intentional conservatism.

TOCTOU window in build_inline_thread_context
(src/safeoutputs/add_pr_comment.rs:262)

There's a small race between resolved_path.canonicalize() and the later std::fs::read_to_string(&canonical) — an adversary with filesystem write access (e.g., a malicious tool in Stage 1 leaving a symlink) could swap the target in that window. In practice Stage 3 runs in an ephemeral agent with only the build workspace mounted, so the practical risk is low. Noting it for completeness.

Redundant double-canonicalization of repo_root

ensure_path_within_base (called in execute) already returns a canonical PathBuf. Then build_inline_thread_context calls repo_root.canonicalize() again. The second call is a no-op but slightly misleading. Same for ctx.source_directory which also gets canonicalized twice in the inline-comment path. Minor.

✅ What Looks Good

• Correct UTF-16 offset: target_line.encode_utf16().count() as i32 + 1 correctly handles multi-byte codepoints (emoji → 2 surrogate pairs) and is validated by the new ab😀 test case.
• Double-boundary path check: Checking that the resolved file is within both the repo checkout root and the workspace root is solid defence-in-depth; canonicalize + starts_with on both is the right pattern.
• validate_relative_safe_path for repository selector: Blocking ../sibling-repo at validation time (with a test) is the right approach; path traversal caught before filesystem access.
• find_matching_fence_end correctness: Using count >= min_count (not ==) for the closing fence correctly handles over-long closers, matching the CommonMark spec.

Generated by Rust PR Reviewer for issue #1104 · 136.8 AIC · ⌖ 13.2 AIC · ⊞ 33.8K · ◷
_{Comment /rust-review to run again}

@copilot address suggestions

@copilot address suggestions

Addressed in dda7427.